Skip to content

Conversation

@MathiasVP
Copy link
Contributor

Magic was giving us the dreaded join-on-index in Function::getParameter. Sadly, the person who was getting this issue didn't save an evaluator log for me, but they did manage to share this very useful screenshot 😅

image (6)

@MathiasVP MathiasVP requested a review from a team as a code owner November 17, 2025 19:26
Copilot AI review requested due to automatic review settings November 17, 2025 19:26
@github-actions github-actions bot added the C++ label Nov 17, 2025
Copilot finished reviewing on behalf of MathiasVP November 17, 2025 19:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds nomagic pragmas to two parameter-related predicates in the Function class to prevent inefficient join operations. The change addresses a performance issue where magic optimization was causing a join-on-index problem in Function::getParameter.

Key Changes

  • Added pragma[nomagic] to Function::getParameter(int n) to prevent magic optimization from creating inefficient join patterns
  • Added pragma[nomagic] to Function::getAParameter() for consistency and to prevent similar performance issues

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@MathiasVP MathiasVP marked this pull request as draft November 17, 2025 19:28
Copy link
Contributor

@jketema jketema left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible. I'm happy if DCA is happy.

@MathiasVP MathiasVP added the no-change-note-required This PR does not need a change note label Nov 17, 2025
@bdrodes
Copy link
Contributor

bdrodes commented Nov 17, 2025

Sorry... I am that person who didn't have the log :-) My editor OOMed during the experiments and I think it wiped all the logs.

@MathiasVP MathiasVP marked this pull request as ready for review November 18, 2025 00:12
@MathiasVP
Copy link
Contributor Author

DCA was uneventful. At first I thought there was a large speedup, but then I realized it was on the macos project only 😭

@MathiasVP MathiasVP merged commit b90d0fd into github:main Nov 18, 2025
24 of 25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C++ no-change-note-required This PR does not need a change note

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants